-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
io: enable closed event for listener filter #21585
Conversation
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
/wait Running some tests on the CI first. |
/retest |
Retrying Azure Pipelines: |
if (events & FileReadyType::Closed && injected_activation_events_ & FileReadyType::Read) { | ||
// We never ask for both early close and read at the same time. If close is requested | ||
// keep that instead. | ||
injected_activation_events_ = injected_activation_events_ & ~FileReadyType::Read; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure why the windows remove the read event, not sure there is the reason why windows can't handle both read and closed events. at least the CI works.
If nobody knows the history or background, a safer way is only register both read and closed events for non-Windows platforms, just silent ignore closed events when both read and closed events registered for Windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davinci26 do you remember the history of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was an implementation detail based on how event registration worked at that point on Envoy and it wasnt a windows issue per se.
@rectified95 works now at Microsoft so he should be able to test event registrations and which events get raised.
@@ -149,15 +143,6 @@ void FileEventImpl::registerEventIfEmulatedEdge(uint32_t event) { | |||
void FileEventImpl::mergeInjectedEventsAndRunCb(uint32_t events) { | |||
ASSERT(dispatcher_.isThreadSafe()); | |||
if (injected_activation_events_ != 0) { | |||
// TODO(antoniovicente) remove this adjustment to activation events once ConnectionImpl can | |||
// handle Read and Close events delivered together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this comment said, this code is for ConnectionImpl
and can't work with both read and close events. So I assume both read and close event should work for listener filter.
Also, note we don't have to fix this if we think this corner case doesn't worth taking the risk. Just give it a try if it looks workable. |
My understanding is that this issue is not deterministic and is just specific to Windows. If we do not have data that the Windows issue was addressed, then it seems to be unwise to revert this fix. |
The issue #18265 (comment) is deterministic. And this isn't specific for Windows. The original PR #18265 is Windows-specific, without that PR, Windows will be stuck when the client sends the data slowly. Since the Windows will remove the After PR #18265 merged, we got the issue this PR is trying to fix. At least Linux has this issue. @ggreenway and I have discussed about this, If the original fix makes the Windows works just except this corner case, it can be leave there. But still give a try, since I think the Windows workaround about slient remove Read event is for I'm thinking another safer way to fix is that just ignore the close event for Windows, but keep registering both read and close events for other platforms. Then we can fix this issue for other platforms, and keep untouch for Windows since wasn't very sure why Windows want to remove Read event sliently. |
/assign @yanavlasov |
/assign @ggreenway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for the case that this fixes (client closes connection before listener filter has read enough data, but after it has written some bytes). I want to see if that test passes on windows in CI.
I'd also like to hear from @davinci26 or @wrowe or another windows expert to see if they recall the history of the behavior on windows.
/wait
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Thanks, done. I thought I have the test and close event handle in the existing code, but I'm wrong. Add them in the new commit. |
/retest |
Retrying Azure Pipelines: |
Emm...it is a real failure on macos. @ggreenway do you know if this is a different behavior on macos? https://dev.azure.com/cncf/envoy/_build/results?buildId=110814&view=logs&j=cd7972b7-e5f6-5989-198f-f5f1ebdf95eb&t=e7ce6244-e6c5-5515-2efb-17f8c18c9157&l=13022 |
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu I looked through the libevent source. The feature we're wanting is |
Thanks for the help! Let me add a check for EV_FEATURE_EARLY_CLOSE |
I'm not sure if we want to start sprinkling libevent /wait |
got it, we have the unittest to check it is support or not when an new platform added (although it is rare case), so it should be fine. Also note, the |
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
// `FileReadyType::Closed` is required `EV_FEATURE_EARLY_CLOSE` feature for libevent, and this feature is | ||
// only supported with `epoll`. But `MacOS` uses the `kqueue`. | ||
// https://libevent.org/doc/event_8h.html#a98f643f9c9063a4cbf410f519eb61e55 | ||
#if defined(__APPLE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggreenway I'm a little hesitant to add this check, even if we add Closed
event for macOS, it won't break anything. maybe without this check will make the code clear? We can just skip the test for macOS. but let me know your opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check can be moved to the test only. Setting the Closed event doesn't cause any harm on macos I don't think; it just doesn't do anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/wait
// `FileReadyType::Closed` is required `EV_FEATURE_EARLY_CLOSE` feature for libevent, and this feature is | ||
// only supported with `epoll`. But `MacOS` uses the `kqueue`. | ||
// https://libevent.org/doc/event_8h.html#a98f643f9c9063a4cbf410f519eb61e55 | ||
#if defined(__APPLE__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check can be moved to the test only. Setting the Closed event doesn't cause any harm on macos I don't think; it just doesn't do anything.
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu hejie.xu@intel.com
Commit Message: io: enable the closed event for listener filter
Additional Description:
Previously the PR #18265 removed the
Closed
event for listener filter, since the Windows doesn't work well.@ggreenway was found there is still have a corner case that can't be handled without the
Closed
event #18265 (comment)This PR tries to introduce the
Closed
event back to the listener filter. As the comments on Windows code said, bothRead
andClosed
can't be registered at same time due to theConnection
doesn't support that yet. That means it shouldn't be a problem for the listener filter. Also currently, there is no Envoy code using bothRead
andClosed
event, so it should be ok to remove those workaround code to make the bothRead
andClosed
events registered to work with listener filter.The full analysis is here #18265 (comment)
Risk Level: high
Testing: unittest should be added
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: Windows platform code changed.